feature(pedm): SQL initialization by allan2 · Pull Request #1302 · Devolutions/devolutions-gateway

Allan Zhang (allan2) · 2025-04-08T23:57:14Z

adds version tracking in the pedm_schema_version table
adds table initialization
- error checking is strict and initialization only happens if the version table is not found
adds SQLite error handling for microsecond timestamp conversion to/from
Chrono
fixes a bug in the libSQL schema where datetimes were stored incorrectly
- they were being stored fractionally, instead of the full microseconds since epoch
adds /about endpoint to get basic info about the running application
- I used this to ensure that the libsql date handling is functional

A few other changes are included:

libsql feature set is changed (default is now false, "stream" removed)

This PR includes all commits from PR #1301, so an option is to merge this one only instead of #1301 first.

- `serve` arguments are changed - it now takes the `Config` struct instead of a path - pipe name is now part of `Config` rather than a direct argument - `Config` struct is now exported as `devolutions_pedm::Config` - it can be used by server implementations directly - `serve` is now exported as `devolutions_pedm::serve`

This commit makes it public again. It is used by the generate-openapi tool in the `devolutions_pedm::api::openapi` call.

- adds version tracking in the `pedm_schema_version` table - adds table initialization - error checking is strict and initialization only happens if the version table is not found - adds SQLite error handling for microsecond timestamp conversion to/from Chrono - fixes a bug in the libSQL schema where datetimes were stored incorrectly - they were being stored fractionally, instead of the full microseconds since epoch - adds `/about` endpoint to get basic info about the running application - I used this to ensure that the libsql date handling is functional A few other changes are included: - libsql feature set is changed (default is now false, "stream" removed)

This adds changes added to PR #1301.

github-actions · 2025-04-08T23:57:29Z

Let maintainers know that an action is required on their side

Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)
Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release
Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)
Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries

Desired changes were removed in the Cargo.toml reversion commit. They are restored now. A few `mod` statements are moved outside of the Windows-only block.

SQLite stores integers in 64 bit. If we try to get i32, it is just [cast internally](https://docs.rs/libsql/0.9.2/src/libsql/rows.rs.html#169-177) as i32. The logic for fallible int conversion is already written. This commit makes use of it and makes it explicit that we are actually retrieving an i64 from the database before converting it to our model type.

Benoît Cortier (CBenoit) · 2025-04-09T02:46:22Z

question: Is there some rationale for when the pedm prefix is used in table names?

It was in case the database is also used by Gateway or other programs in the future.

I see what you mean. The table naming is inconsistent. And I can't see other programs using this specific http_request table.

Tables are renamed in 72bae7b. The database is meant for isolated PEDM usage. This is expressed in the example config, with a database name of pedm or file of pedm.sqlite.

I think it’s better to use a different database completely! 🙂
I agree with the way you renamed the tables!

`pedm_` prefix is removed. The usage was inconsistent. `pedm_schema_version` is now simply `version`. The database is meant for isolated PEDM usage. This is expressed in the example config, with a database name of `pedm` or file of `pedm.sqlite`.

- use `DateTime` `Display` instead of `Debug` in `ParseTimestampError` `Display` - comments should go above `#[cfg(feature)]` - adds random comments

Postgres `log_http_layer` was calling `query_one` when nothing was returned. The database was still written but the function returned an error.

This one was a bit funny. `query_opt` returns `Option<Row>`. It was doing `opt_row.map(|r| r.get::<_, Option<D>(0))`, which would be `Some(Some(D))` or `None` if the row was not found. With `unwrap_or_default`, this was returning `Some(D)` or `None`, which is perfectly well. However, the actual column is not nullable so `opt_row.map(|r| r.get::<_, D>(0))` is simpler.

This disambiguates `requests_received` and `request_count`.

Benoît Cortier (CBenoit) · 2025-04-10T11:37:50Z

-    pub(crate) request_count: i32,
+    /// The request count at the time of the server startup.
+    pub(crate) startup_request_count: i32,


naming: I feel it’s not necessary to add the startup prefix, since this is a field of a struct named StartupInfo. Or maybe I don’t really understand what this is? Is the intention to count the total of requests ever made? "All time request count"?

It was because I was using the field in #[serde(flatten)]. But evidently the naming still wasn't clear.

My proposal is c17bdc5.

Benoît Cortier (CBenoit) · 2025-04-10T11:44:25Z


 mod err;

+use chrono::{DateTime, Utc};


style: Inconsistent import order.

Addressed in 61960d1#diff-00e7d35440ea7e484c0981392b0a300237b094ff70efeb677cee4fb73e90b675R6.

Benoît Cortier (CBenoit) · 2025-04-10T11:48:53Z

+    /// Returns the schema version from the `version` table.
+    async fn get_schema_version(&self) -> Result<i16, DbError>;
+
+    /// Initializes the database schema.
+    ///
+    /// This creates tables.
+    async fn init_schema(&self) -> Result<(), DbError>;


question: Should we really expose the schema version to the user of the Database trait?

I think it’s more appropriate to define a single method called setup that is performing any backend-dependent setup sequence, including initializing the schema and/or performing migrations if necessary.

See how it’s done in job-queue/job-queue-libsql crates:

devolutions-gateway/crates/job-queue-libsql/src/lib.rs

Lines 174 to 178 in cdbe0b7

async fn setup(&self) -> anyhow::Result<()> {

self.apply_pragmas().await?;

self.migrate().await?;

Ok(())

}

My proposal is 4eed892.

I put shared logic in the Db wrapper setup function, formerly called init_schema.

I kept get_schema_version and init_schema on the Database trait because they are just DB operations.

I didn't do Database::setup because I didn't want to duplicate the code in Db::setup. But I can make Database::setup with the duplication if you prefer to keep the logic in the trait implementation.

Fair enough. My thinking was to keep the exact setup logic as an implementation detail to leave more leeway, but at this point it does not really matter so we may as well avoid the logic duplication as you suggest. Certainly a better abstraction will emerge when it becomes necessary.

Benoît Cortier (CBenoit)

thought: It seems the /about route is close to something that is typically called /health. I’m curious if you have further motivations for the /about route?

Benoît Cortier (CBenoit) · 2025-04-10T13:53:41Z

+    seconds integer NOT NULL
+);
+
+INSERT INTO version (version) VALUES (1) ON CONFLICT DO NOTHING;


question: Any objection to starting with 0? I think it can be useful in the future, so we can index an array of migrations.

I just chose 1 because SQL sequences start at 1.

Let me know what you prefer and I'll change it.

I think 0 would be better because we wouldn’t index into a SQL sequence. But, it’s not a hill I’m willing to die on either as it’s not too hard to throw a - 1 in there.

Changed to 0 in 2203fb9.

This further disambiguates `startup_request_count` and `current_request_count` in `AboutData`.

I went through every file in the crate.

This adds a placeholder for backend-specific setup code, such as pragma tuning on SQLite.

Allan Zhang (allan2) · 2025-04-10T15:27:55Z

thought: It seems the /about route is close to something that is typically called /health. I’m curious if you have further motivations for the /about route?

/health is an infallible endpoint to check if the server is running.
/about could fail because of a DB error.

As for the motivation, I included some info that was useful for debugging.

Benoît Cortier (CBenoit)

LGTM! Feel free to merge!

Allan Zhang (allan2) added 5 commits April 8, 2025 15:08

Fix Linux build

c75a0df

Fix api module publicity

f53a93c

This commit makes it public again. It is used by the generate-openapi tool in the `devolutions_pedm::api::openapi` call.

Merge branch pedm-sql-init into pedm-sql-init3

70b2349

This adds changes added to PR #1301.

Allan Zhang (allan2) added 6 commits April 8, 2025 19:58

Unformat Cargo.toml

941ec36

Update Cargo.lock

7327209

Restore Cargo.toml; fix merge issue

d0a562c

Desired changes were removed in the Cargo.toml reversion commit. They are restored now. A few `mod` statements are moved outside of the Windows-only block.

Update Cargo.lock

8ddcbdd

Correct typo

f477e90

Benoît Cortier (CBenoit) self-requested a review April 9, 2025 01:19

Benoît Cortier (CBenoit) reviewed Apr 9, 2025

View reviewed changes

Allan Zhang (allan2) added 5 commits April 9, 2025 00:37

Merge origin/master

bb2b9d7

Remove extra line

5869fca

Rename tables

72bae7b

`pedm_` prefix is removed. The usage was inconsistent. `pedm_schema_version` is now simply `version`. The database is meant for isolated PEDM usage. This is expressed in the example config, with a database name of `pedm` or file of `pedm.sqlite`.

Style: variable name

d52f5b3

fmt

8d01f31

Benoît Cortier (CBenoit) reviewed Apr 9, 2025

View reviewed changes

Comment thread crates/devolutions-pedm/src/api/mod.rs Outdated

Allan Zhang (allan2) added 7 commits April 9, 2025 09:08

Use self:: when importing from current module

49abe22

Minor fmt and doc changes

33956e5

- use `DateTime` `Display` instead of `Debug` in `ParseTimestampError` `Display` - comments should go above `#[cfg(feature)]` - adds random comments

fmt

332df23

Fix bug in LogLayer

65f36c0

Postgres `log_http_layer` was calling `query_one` when nothing was returned. The database was still written but the function returned an error.

Rename request count in /about

9d82ae7

This disambiguates `requests_received` and `request_count`.

Update Cargo.lock

435fe4b

Benoît Cortier (CBenoit) reviewed Apr 10, 2025

View reviewed changes

Allan Zhang (allan2) added 4 commits April 10, 2025 09:57

Rename request counts in AboutData

c17bdc5

This further disambiguates `startup_request_count` and `current_request_count` in `AboutData`.

Reorder imports

61960d1

I went through every file in the crate.

Rename init_schema to setup on Db wrapper

fd30525

Add pragma placeholder

4eed892

This adds a placeholder for backend-specific setup code, such as pragma tuning on SQLite.

Benoît Cortier (CBenoit) approved these changes Apr 10, 2025

View reviewed changes

Change initial schema version to 0

2203fb9

Allan Zhang (allan2) enabled auto-merge (squash) April 10, 2025 16:26

Allan Zhang (allan2) merged commit 76138e9 into master Apr 10, 2025
38 checks passed

Allan Zhang (allan2) deleted the pedm-sql-init3 branch April 10, 2025 16:51

	async fn setup(&self) -> anyhow::Result<()> {
	self.apply_pragmas().await?;
	self.migrate().await?;
	Ok(())
	}

Conversation

Allan Zhang (allan2) commented Apr 8, 2025

Uh oh!

github-actions Bot commented Apr 8, 2025

Let maintainers know that an action is required on their side

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Allan Zhang (allan2) Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Allan Zhang (allan2) commented Apr 10, 2025

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Benoît Cortier (CBenoit) Apr 9, 2025 •

edited

Loading

Benoît Cortier (CBenoit) Apr 10, 2025 •

edited

Loading

Allan Zhang (allan2) Apr 10, 2025 •

edited

Loading

Benoît Cortier (CBenoit) Apr 10, 2025 •

edited

Loading